Size Change: -1.28 kB (0%)

Total Size: 1.19 MB

_{compressed-size-action}

It's expected this may also result in a smaller overall bundle size, as we are not using the full extent of the feature set of qs.

Also seen in the above #20693 (comment), there is a pretty nice drop. The results of npm run build before and after these changes are a difference of 11.1kb -> 5.5kb for the url bundle.

Relevant comment at #20738 (comment) about the commit 8dfe648 here.

As seen with the changes in 0a6046a, the new implementation is slightly more strict in regards to assuming the input argument to be a proper URL, not merely any string which happens to contain ?. While this could limit flexibility, I believe it's an appropriate change given:

Note that I had to scale this back a bit in #20693. Specifically, there's at least a few occurrences where we rely on an implementation like this:

addQueryArgs( '/wp-json/wp/v2/posts`, { type: 'post' } );

And if @wordpress/url only supports full URLs, it loses a lot of its usefulness.

The solution (8dfe648) is a bit hacky, effectively treating the input URL as the path of a throwaway base (second argument of URL constructor).

It's something I think we should want a consistent approach for in this module. Certain functions, we clearly want to expect a full URL (like isValidURL). Others, like getQueryString, addQueryArgs, removeQueryArgs, etc, we might want to be more tolerant about what's received, notably to support use-cases like in the quoted text above where we might want to be able to trigger fetch requests using an root-relative path.

Maybe the compromise is that for such functions, we specifically detect if the input is a root-relative URL and, if it is, construct URL with a fake base.

e.g.

let base;
if ( url.startsWith( '/' ) ) {
    base = 'https://example.com';
}

const urlInstance = new URL( url, base );

Maybe the compromise is that for such functions, we specifically detect if the input is a root-relative URL and, if it is, construct URL with a fake base.

e.g.

let base;
if ( url.startsWith( '/' ) ) {
    base = 'https://example.com';
}

const urlInstance = new URL( url, base );

There's some prior art for this approach by @sgomes. See https://github.com/Automattic/wp-calypso/blob/397d7592efea16154218b424602b15db64063361/client/lib/url/format.ts#L49 for an example.

There's some prior art for this approach by @sgomes. See https://github.com/Automattic/wp-calypso/blob/397d7592efea16154218b424602b15db64063361/client/lib/url/format.ts#L49 for an example.

Calypso's lib/url will likely become an independent package at some point. I expect it to have no dependencies but require a URL/URLSearchParams implementation present, either natively or through a polyfill.

I'm happy to accelerate any efforts in packaging it up if anyone's planning to reuse it!

@sirreal @sgomes That's interesting, thanks for sharing! I could definitely see some value in some of this, such as detecting the "type" of a URL. I think at least for the immediate purposes here, the current implementation proposed here more-or-less matches how it's implemented in Calypso's lib/url, though perhaps relying a little too much on the URL constructor to do the heavy lifting.

For me, it's largely a question of documentation. Some of the functions of this module demand to operate only with a full URL (isValidURL, getQueryString as of #20738 and then reverted here). In the case of isValidURL, it makes sense. So the uncertainty I have now is around (a) determining whether isValidURL is the sole exception, (b) documenting these expectations, and (c) implementing those expectations consistently.

I just realized this pull request was left with the "In Progress" label. As far as I can recall, this is complete and ready for review.

@sirreal, any idea why TypeScript errors:

Error: packages/url/src/add-query-args.js(5,34): error TS6307: File ‘/home/runner/work/gutenberg/gutenberg/packages/url/src/build-query-string.js’ is not listed within the file list of project ‘/home/runner/work/gutenberg/gutenberg/packages/url/tsconfig.json’. Projects must list all files or use an ‘include’ pattern.

FYI @sgomes - removing qs dependency when working with URLs.

There is one failing e2e test but I saw it before in other PRs and it doesn't seem to be relevant.